Skip to content

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Sep 4, 2025

No description provided.

@dgryski dgryski force-pushed the dgryski/http-downstream branch 4 times, most recently from 8ce4757 to ff659ed Compare September 5, 2025 20:38
@dgryski dgryski marked this pull request as ready for review September 5, 2025 20:56
@dgryski
Copy link
Member Author

dgryski commented Sep 10, 2025

I wonder if I should name the Fingerprint struct to FastlyMetadata or something to match more closely with the Rust SDK.

@cee-dub
Copy link
Collaborator

cee-dub commented Sep 12, 2025

Maybe update this branch to use withAdaptiveBuffer?

@dgryski
Copy link
Member Author

dgryski commented Sep 12, 2025

Yes, I'll update this to use the new code after your PR is merged.

@dgryski dgryski force-pushed the dgryski/http-downstream branch from e4e10f1 to b62155f Compare September 12, 2025 19:48
@dgryski
Copy link
Member Author

dgryski commented Sep 12, 2025

Branch updated. Any thoughts on renaming req.Fingerprint() and friends or is this good to merge? (I guess we still have time until we tag a release to change this.)

@dgryski
Copy link
Member Author

dgryski commented Sep 12, 2025

Added some comments back.

Comment on lines 387 to 395
fingerprint.DDOSDetected, err = req.abi.req.DownstreamDDOSDetected()
if err != nil {
return nil, fmt.Errorf("get ddos detected: %w", err)
}

fingerprint.FastlyKeyIsValid, err = req.abi.req.DownstreamFastlyKeyIsValid()
if err != nil {
return nil, fmt.Errorf("get fastly key is valid: %w", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly like these two fields inside Fingerprint since they don't seem related.

Comment on lines 1055 to 1056
// FastlyKeyIsValid is true if the request contains a valid Fastly API token.
FastlyKeyIsValid bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do customers' apps need to know this?

@dgryski
Copy link
Member Author

dgryski commented Sep 15, 2025

PTAL.

Copy link
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dgryski dgryski merged commit 3eac1c6 into main Sep 15, 2025
10 checks passed
@dgryski dgryski deleted the dgryski/http-downstream branch September 15, 2025 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants